-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Http api #2253
Http api #2253
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NACK, particularly with regard to the dependency concerns below.
My understanding is that this PR supersedes #2240. Not sure why that one hasn't been closed in favor of this one.
I have limited time and so have not done anything approaching a complete review of this work, but here's what I see from the few minutes I've given to it:
-
There are a large number of dependencies introduced here. Has every effort been made to reduce these? Why for example is the Kotlin standard library included here? Is it actually required at compile or runtime, or is this (large) dependency actually optional but being being dragged in by default nevertheless? Manfred and I agreed some time ago to take a stricter stance with the introduction of new dependencies. There are already so many, and each is a potential attack vector. This is not to say the solution should be fundamentally reworked so as to reduce these dependencies, but that the dependencies themselves should be closely scrutinized so as to eliminate all those that are not absolutely necessary to the functioning of the 'minimal http api'
-
I would reconsider the
bisq.httpapi
packaging, and rename it tobisq.http
. "httpapi" is awkward, IMO, and 'http' is clear enough, i.e. this package and its subpackages are about an HTTP interface to a running Bisq node. I don't think we need to explicitly qualify with "API" everywhere. Likewise, I would name the gRPC package simplybisq.grpc
, notbisq.grpcapi
. -
Import organization needs some work. Add entries to the
IMPORT_LAYOUT
section of.idea/codeStyles/Project.xml
to appropriately order new packages. Keep in mind that the ordering here is not alphabetical, but rather based on each package's relationship to other packages in the overall stack, i.e. how "high or low level" the dependency is. This is not an exact science, but something that you eyeball and get approximately correct. The idea is to be able to read the import statements in any given class definition in a logical and useful way. They should read from top to bottom with high-level packages, e.g.bisq.*
being at the top, and low-level packages, e.g.java.*
being at the bottom, with significantly different packages being separated by blank lines where appropriate. -
I don't see any documentation here or links to or mentions of plans for documentation elsewhere. Minimally, I would expect a
README.md
in the newhttp-api
subproject directory that details what, at a minimum, is necessary to get up and running with the API (command line args, etc), and a simple-as-possible 'hello world' example of getting results from it, ideally usingcurl
at the command line (possibly in conjunction withjq
), such that anyone can just try the example out without having to bootstrap a particular programming environment to do so. Ideally, a larger worked example would be documented as well, e.g. something approaching the "simplest possible trading bot", but I agree it would be reasonable to do that after the inclusion of this most basic cut at the API.
Again, the above are my observations after merely looking through the PR textually. I have not spun up the API, attempted to use it, reviewed the structures and data that are produced by it, etc.
As a final and perhaps rather frank note, I am just not excited about adding a bespoke HTTP+JSON API to Bisq. As has been discussed elsewhere over the last year or so, a (g)RPC API would be preferable for the creation of trading bots and most other applications that interact programmatically with a Bisq node, and with the GA release of gRPC-Web late last year (announcement, docs here and here, GitHub project), I see no reason not to consolidate up front on gRPC as it appears to be able to handle all API use cases now. In my opinion, an HTTP API such as the one presented here will quickly become legacy following the introduction of a proper gRPC API, and will just mean that much more maintenance burden. I know I would want to deprecate and remove it as soon as we have a working gRPC API. I've mentioned this before, but we can learn from the Lightning Labs team on this front: they view the lnd
HTTP API as a legacy burden and direct users by default to the (newer) gRPC API by default. I understand a lot of work (and re-work) has gone into this HTTP API over the last two years, but this kind of "sunk cost" reasoning is not sufficient IMO to justify its inclusion if we have every expectation that it's going to create even more costs and burden down the road, i.e. if we can predict that we'll want to get rid of it as soon as a gRPC API exists. I won't go so far here as "vetoing" the inclusion of this API, as it could be of value to users in the interim, so long as we're agreed that we may deprecate and remove it in the relatively near future. I wouldn't want to create or imply any strong backward compatibility guarantees around this work if we can already see its successor on the horizon.
dependenciesI'll try to remove each transitive dependency and run tests to see if they are really required.
|
@ManfredKarrer why did you remove integration tests from travis? They are the best testing effort we've got. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK api works in both headless and desktop mode, integration tests are passing.
@ManfredKarrer I've tried trimming the dependencies for the api. Most were required. |
hi @cbeams, thx for the response, my 2cents (as @blabno has been doing most of the work lately) on the http/grpc discussion - most of the work in getting the http api merged is actually reworking bisq itself. Once the bisq code is 'api ready', integrating grpc would be relatively straightforward. Anyone can whip up the grpc code, but getting the bisq code to the point that it's api-suitable without breaking things is long and hard work that manfred and blabno are doing right now, adding grpc into the mix would just be a distraction in my opinion. Even after gRPC is available an http api will probably be useful for a subset of users. If the grpc-web thing works well then the current http api could indeed be replaced by it, why not. |
Thanks @cbeams for the review! I will discuss with @blabno about grpc. As you might be surprised why we created a new branch next to master for the api (as well for the new trade protocol) here is my reasoning behind that: Re Kotlin: Re bisq.httpapi: Re codeStyles: Re docs: |
@blabno re travis, lets discuss in a call. I have a few questions reagarding that... |
@blabno
Let me know once you had time to investigate in grpc. I would independently just go on with that branch and if we decide to move directly to grpc we can delete that branch. |
@ManfredKarrer PR ready here: ManfredKarrer#1 Headless issue is more complex and I lack knowledge about the startup process. I think that the problem is located somewhere around https://github.com/bisq-network/bisq/blob/master/core/src/main/java/bisq/core/app/BisqSetup.java#L475 |
@blabno @cbeams @cbeams I discussed with @blabno to investigate effort for moving directly to grpc. It will probably take a bit until he can report something. In the meantime I would prefer to go on with the currnet http api version. |
@ManfredKarrer Just add following tweaks to readme: ManfredKarrer#1 |
@blabno Ah good. I will merge then. |
No description provided.